refactor(parser): Avoid peek in parse_delimited_list#11343
Merged
graphite-app[bot] merged 1 commit intomainfrom May 28, 2025
Merged
refactor(parser): Avoid peek in parse_delimited_list#11343graphite-app[bot] merged 1 commit intomainfrom
graphite-app[bot] merged 1 commit intomainfrom
Conversation
Contributor
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
github-actions
bot
added
A-parser
CodSpeed Instrumentation Performance ReportMerging #11343 will improve performances by 14.65%Comparing Summary
Benchmarks breakdown
|
leaysgur
force-pushed
the
r/delimited-list
branch
from
44ee2be to
8edef9f
Compare
Member
Author
|
At this point, there appears to be no regression in the parser benchmark. But strangely, when I added a commit that removes the unused ...🤔 |
Contributor
There was a problem hiding this comment.
Pull Request Overview
Refactors parse_delimited_list to return a tuple with an optional trailing-separator span, and adds parser-level checks for invalid trailing commas in specific constructs.
- Changed all
parse_delimited_listinvocations to the new signature(Vec, Option<u32>) - Added explicit error reporting for trailing commas in index signatures and parenthesized expressions
- Updated context wrappers to destructure the new tuple return
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_parser/src/ts/types.rs | Switched to tuple return from parse_delimited_list and added trailing-comma error for index signatures |
| crates/oxc_parser/src/ts/statement.rs | Adapted enum-body parsing to the new parse_delimited_list signature |
| crates/oxc_parser/src/js/object.rs | Updated object-expression parsing to destructure the new return |
| crates/oxc_parser/src/js/module.rs | Simplified import/export specifier parsing with the new signature |
| crates/oxc_parser/src/js/expression.rs | Hooked into trailing-comma span for parentheses, arrays, and calls |
| crates/oxc_parser/src/cursor.rs | Changed parse_delimited_list to return trailing-separator info and marked unused peek_at |
Comments suppressed due to low confidence (2)
crates/oxc_parser/src/ts/types.rs:1233
- There should be a unit test covering the new trailing-comma error path in index signatures to ensure this diagnostic fires when a comma precedes
].
if let Some(comma_span) = comma_span {
crates/oxc_parser/src/js/expression.rs:212
- Add tests to validate that a trailing comma inside parentheses triggers the
fatal_errorpath and correctly reports the unexpected comma.
if let Some(comma_span) = comma_span {
Member
Merge activity
|
Out of curiosity, I've just deleted the peeking in `parse_delimited_list`. Then, coverage errors show me there are only a few places missing error report for invalid trailing comma after rest element, etc. So I added those checks manually. In TSC, these checks seem to be done at checker, not parser. But we need this in parser for JS. --- (Sorry, I have PASSED the test, but I am not very confident that this change is intrinsically sound.)
graphite-app
bot
force-pushed
the
r/delimited-list
branch
from
8edef9f to
069b843
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Out of curiosity, I've just deleted the peeking in
parse_delimited_list.Then, coverage errors show me there are only a few places missing error report for invalid trailing comma after rest element, etc.
So I added those checks manually.
In TSC, these checks seem to be done at checker, not parser. But we need this in parser for JS.
(Sorry, I have PASSED the test, but I am not very confident that this change is intrinsically sound.)